-
Notifications
You must be signed in to change notification settings - Fork 168
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FIX] Do not require notch frequencies to be parsed as numbers, accommodating multiples #1605
Conversation
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## master #1605 +/- ##
=======================================
Coverage 87.83% 87.83%
=======================================
Files 16 16
Lines 1356 1356
=======================================
Hits 1191 1191
Misses 165 165 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments cuz this has been an issue for me in the past. Hopefully it helps!
d28eff0
to
3346056
Compare
@adam2392 Are you comfortable with the current wording leaving open the possibility of a more structured recommendation/requirement in the future? |
The [low, center, high] is incorrect. The line noise is at 60Hz, so that is where you apply a notch. You sometimes also apply a notch filter to the harmonics at 120 and 180 Hz. Therefore 60Hz should not be referred to as 'low' and 120Hz is definitely not 'center'. |
Thank you for the clarification. Is the current wording better? |
Yes, thank you! Just a minor suggestion is to say: "If notch filters are applied at multiple frequencies, these frequencies MAY ..." |
2bfdb3b
to
296cebc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am aware that this PR is mostly to get an already existing practice finally incorporated into the spec, but it irks me that this is not very in line with:
Do you think we should RECOMMEND specifying the frequencies as a comma separated list like 60, 120, 240
, and clarify that an old method of [60, 120, 240]
is DEPRECATED?
Either way, I am approving this, because I am fine with my suggestion being dealt with in a separate PR. Thanks everyone.
@Andesha is this something you'd feel comfortable taking a moment to review? |
While schematizing, the
notch
column was defined as a number. In the case of iEEG at least, the BEP leads had submitted examples wherenotch
took the form[F1, F2, F3]
. This PR recognizes this as existing practice and suggests that curators and tools adopt[F1, F2, ...]
for interoperability, but does not require it.cc @bids-standard/raw-electrophys for comment.
Closes bids-standard/bids-examples#395.